Fix UB on empty trampoline template entries#112
Conversation
| T->num_entries = num_entries; | ||
| memcpy(T->entries, &entries[0], num_entries * sizeof(Entry)); | ||
| if (num_entries > 0) | ||
| memcpy(T->entries, entries.data(), num_entries * sizeof(Entry)); |
There was a problem hiding this comment.
Hi, just want to note that the branching is not necessary here (and on whether it's desirable, I don't have any informed opinion).
There was a problem hiding this comment.
The error message in my description is from libstdc++ debug mode. I also checked with UBSan, and it shows an error message for this exact same access. Accessing index 0 of an empty vector is an Undefined Behavior according to the C++ standard.
I agree that this bug might not be very critical in real situations. However, considering memory safety, I think it is better to keep this branch to prevent the UB explicitly.
There was a problem hiding this comment.
I think @McSinyx was just pointing out that the branch if (num_entries > 0) could be technically removed from the patch, since if num_entries == 0 then entries.data() is still defined and memcpy is a nop. The original code was clearly UB however.
|
Thanks for the report and patch. @McSinyx: you are right, but it's fine either way. |
3 participants
Description
This PR fixes an undefined behavior in empty trampoline template handling.
Root Cause
When template is empty ([]),
entriesin e9json.cpp:201 can be empty, but code still accessedentries[0]in trampoline duplication. (e9json.cpp:210) That is out-of-bounds access (UB).Reproduction
make clean && CXXFLAGS='-D_GLIBCXX_DEBUG' make -j$(nproc) debug./e9tool --backend ./e9patch -M 1 -P empty <binary>Before fix: abort with vector out-of-bounds message.
Fix
num_entries > 0.entries[0]when entries is empty.